-
Notifications
You must be signed in to change notification settings - Fork 5.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
*: Support for caching_sha2_password authentication #24991
Conversation
Please follow PR Title Format:
Or if the count of mainly changed packages are more than 3, use
|
No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md |
default: | ||
return errors.New("Unknown auth plugin") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could potentially use an authSwitchRequest
(from deleted code above) if the plugin is unknown. The downside is that this is hard to test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the client presents an authentication plugin that doesn't match the plugin used by the server checkAuthPlugin
will do the authSwitchRequest
so I don't think this code will be reached during normal operation. It may be reached if someone bypasses the GRANT
/REVOKE
etc and changes mysql.user
directly.
ddd7e50
to
517069e
Compare
517069e
to
1b57a5d
Compare
1b57a5d
to
9b5bd45
Compare
9b5bd45
to
1246b7a
Compare
return nil | ||
} | ||
|
||
func (cc *clientConn) openSessionAndDoAuth(authData []byte) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If open session success, but do auth fails, will there be a potential resource leak?
@@ -715,6 +775,32 @@ func (cc *clientConn) openSessionAndDoAuth(authData []byte) error { | |||
return nil | |||
} | |||
|
|||
func (cc *clientConn) checkAuthPlugin(ctx context.Context, authPlugin *string) ([]byte, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, there are:
- the default auth plugin (we set in the initial handshake packet)
- the user's auth plugin, which stores in mysql.user authentication related column
- the auth plugin from the client response packet
Which one is which? I'm a bit confused.
Pass *string as both input and output of a function is an idiom in C.
Go support multiple return, you don't need to do that.
1246b7a
to
05b10b4
Compare
/assign @tiancaiamao @morgo |
if err != nil { | ||
return err | ||
} | ||
cc.ctx = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a hint: open a new session may need to load the global variables (maybe not true when they're cached), and do it once again could slow down the connection speed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with your comment.
The cache is improved with sysvar cache, but it is still expensive. The current code will re-run validation on all of the sysvars. This can be improved though: in future we can cache the validated values in the sysvar cache, and when a session is initialized just copy the sysvar cache map to it.
The complicated case will be eliminating the need to call the 'init functions' (i.e. calling sv.SetSession()
so that s.VarXXX
is set.) I think we can remove a lot of SetSession()
funcs by adding something like sv.ToBool()
, sv.ToUint64()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, creating a new session for handshake is relatively heavy, indirectly leading to the occurrence of #44502.
@morgo would you like to take another look? or I'll merge this one. |
It LGTM. There are some known issues with improving fast connection/disconnection rate, but these are existing. We should suggest the QA team have scenario testing for it because it will affect connectors like php and perl. Issues like #24326 could also be discovered quicker. |
/merge |
This pull request has been accepted and is ready to merge. Commit hash: e2e9861
|
@dveeden: Your PR was out of date, I have automatically updated it for you. At the same time I will also trigger all tests for you: /run-all-tests If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
Related to pingcap/tidb#24991 Co-authored-by: TomShawn <41534398+TomShawn@users.noreply.github.com>
Related to pingcap/tidb#24991 Co-authored-by: TomShawn <41534398+TomShawn@users.noreply.github.com>
Related to pingcap/tidb#24991 Co-authored-by: TomShawn <41534398+TomShawn@users.noreply.github.com>
Related to pingcap/tidb#24991 Co-authored-by: TomShawn <41534398+TomShawn@users.noreply.github.com>
What problem does this PR solve?
Issue Number: close #9411
Problem Summary:
mysql_native_password
uses the SHA1 hash algorithm which is not future proofcaching_sha2_password
from MySQL to TiDB doesn't workWhat is changed and how it works?
What this does:
plugin
column of themysql.user
table.sent we may need to switch the authentication plugin to match the
one from the user record
caching_sha2_password
send the "fastauthentication failed" response to trigger full authentication.
auth.CheckShaPassword
to validate the user.Implemented functionality:
caching_sha2_password
over TLSdefault_authentication_plugin
variableCREATE USER... IDENTIFIED WITH 'caching_sha2_password'...
Missing functionality:
Related changes
of this easier, but this is not required.
Tests
Create users with:
mysql_native_password
caching_sha2_password
Try to authenticate with all of them.
Side effects
Release note
caching_sha2_password
authentication